[CONSULT-469] - introduce macro for PGN registration and NmeaMessage type #408

gvaradarajan · 2025-01-30T21:12:32Z

There was some confusion around source id that was cleared up with further testing. It turns out that source_id from some messages and source from the message metadata do not necessarily match (and that some messages do not even have a source_id). This PR contains the subsequent fix

…type

acmorrow

Lots of thoughts and questions on this one

micro-rdk-nmea/src/parse_helpers/parsers.rs

micro-rdk-nmea-macros/src/composition.rs

micro-rdk-nmea/src/messages/pgns.rs

acmorrow · 2025-01-31T18:21:06Z

micro-rdk-nmea/src/messages/pgns.rs

+
+impl NmeaMessage {
+    pub fn new(mut bytes: Vec<u8>) -> Result<Self, NmeaParseError> {
+        let msg_data = bytes.split_off(32);


This 32 should be a constant somewhere, and replaced globally

micro-rdk-nmea/src/messages/pgns.rs

acmorrow · 2025-01-31T18:28:25Z

micro-rdk-nmea/src/messages/pgns.rs

+}
+
+impl NmeaMessage {
+    pub fn new(mut bytes: Vec<u8>) -> Result<Self, NmeaParseError> {


I'm curious why this takes the Vec by value rather than just viewing the data, since ultimately, everything gets parsed out, shouldn't this be able to take something like a byte slice, all the way down?

Should this implement TryInto at the top level?

I'm not sure I understand the first question, but I prefer dealing with a vec rather than a byte slice where I would have to consider the lifetime of a higher level piece of data.

I can make this TryFrom<Vec<u8>>

I do see what you are saying about the virality of lifetime when dealing with byte slices, but, on the other hand, Vec is saying that this method needs a uniquely owned buffer of data in order to convert it into an NmeaMessage. That seems like it shouldn't be needed. If I have a bunch of bytes that I read into a buffer off the wire or bus that is, say, already in a Vec, I'd expect the parser to just be able to consume that data by read-only reference, rather than needing to copy it into a Vec just to call this method. On the other hand, I don't want to delay this work further. Would you be OK with filing a follow-up ticket?

I previously had DataCursor work this way, but then @npmenard said it should own the data. If you can convince him this is better when he gets back, I'll make the ticket.

My opinion on this is not strong but I don't really see a use-case outside of debugging to want a read-only reference to the data meant to be converted. I can see a use case for maybe supporting any type that implements Read. I could make a ticket for that if you agree

acmorrow · 2025-01-31T18:32:34Z

micro-rdk-nmea/src/lib.rs

@@ -26,11 +29,10 @@ mod tests {
        let mut data = Vec::<u8>::new();
        let res = general_purpose::STANDARD.decode_vec(water_depth_str, &mut data);
        assert!(res.is_ok());
-        let cursor = DataCursor::new(data[33..].to_vec());
-        let message = WaterDepth::from_cursor(cursor, 13);
+        let cursor = DataCursor::new(data[32..].to_vec());


So, before, what was happening by starting at byte 33?

Also, is there a reason in this test and the others not to interpret the metadata in the 32 bytes at the front? It seems worthwhile to test that it parses correctly.

Finally, it looks like WaterDepth still does have a source_id, so, I'm curious why the value is no longer being validated.

Byte 32 was the source_id for those messages that have one, but not every message has a source_id field. I can add checks in the tests for the ones that do

I think it is worth checking the source_id is decoded correctly for those types that have them.

I also think it is worth parsing the metadata here and validating the decoded values.

Thanks for adding the check of the source_id. Did you disagree with the idea to validate the metadata too?

I forgot about that one, I'd prefer to give it its own unit test rather than add it to all of these?

I don't feel too strongly about it. I was suggesting it mostly because you already have static test data here, but the first 32 bytes get thrown away, when it could be a perfectly good test of decoding the metadata. If you do a separate test then you would just need to obtain the metadata bytes from somewhere anyway. Up to you.

wait I do have a unit test for this actually, does this really matter for every message type?

No. Maybe just do one of them, if you want to.

micro-rdk-nmea/src/messages/pgns.rs

acmorrow · 2025-01-31T20:00:41Z

micro-rdk-nmea/src/messages/pgns.rs

+}
+
+impl NmeaMessage {
+    pub fn new(mut bytes: Vec<u8>) -> Result<Self, NmeaParseError> {


I do see what you are saying about the virality of lifetime when dealing with byte slices, but, on the other hand, Vec is saying that this method needs a uniquely owned buffer of data in order to convert it into an NmeaMessage. That seems like it shouldn't be needed. If I have a bunch of bytes that I read into a buffer off the wire or bus that is, say, already in a Vec, I'd expect the parser to just be able to consume that data by read-only reference, rather than needing to copy it into a Vec just to call this method. On the other hand, I don't want to delay this work further. Would you be OK with filing a follow-up ticket?

micro-rdk-nmea/src/messages/pgns.rs

acmorrow · 2025-01-31T20:03:01Z

micro-rdk-nmea/src/lib.rs

@@ -26,11 +29,10 @@ mod tests {
        let mut data = Vec::<u8>::new();
        let res = general_purpose::STANDARD.decode_vec(water_depth_str, &mut data);
        assert!(res.is_ok());
-        let cursor = DataCursor::new(data[33..].to_vec());
-        let message = WaterDepth::from_cursor(cursor, 13);
+        let cursor = DataCursor::new(data[32..].to_vec());


I think it is worth checking the source_id is decoded correctly for those types that have them.

I also think it is worth parsing the metadata here and validating the decoded values.

micro-rdk-nmea-macros/src/composition.rs

micro-rdk-nmea/src/messages/pgns.rs

acmorrow · 2025-02-03T17:03:53Z

micro-rdk-nmea/src/messages/pgns.rs

+define_pgns!(
+    (WaterDepth, Pgn128267, 128267),
+    (TemperatureExtendedRange, Pgn130316, 130316),
+    (GnssSatsInView, Pgn129540, 129540)


With the enum variants looking like Pgn130316 or Pgn129540, this enum is somewhat illegible to a user, since they need to know the PGN code to match, rather than the human readable name like GnssSatsInView. Is it necessary to have the PGN number in the enum variants? Or could they be named much like the wrapped value?

acmorrow

I think this is very close to ready to approve. This pass is mostly only small things or questions/suggestions, and some naming nits.

micro-rdk-nmea/src/messages/pgns.rs

acmorrow · 2025-02-03T20:38:08Z

micro-rdk-nmea-macros/src/attributes.rs

+                            }
+                            _ => {
+                                return Err(error_tokens(
+                                    "bits received unexpected attribute value",


pgn instead of bits, I think.

micro-rdk-nmea/src/messages/pgns.rs

acmorrow · 2025-02-03T20:45:45Z

micro-rdk-nmea/src/messages/pgns.rs

+        let metadata = NmeaMessageMetadata::try_from(value)?;
+        let data = MessageData::from_bytes(metadata.pgn(), msg_data)?;


I feel like both these types (NmeaMessageMetadata and MessageData) should either start with Nmea, or not. Given that there is also NmeaMessage, maybe that breaks the tie in favor of NmeaMessageData?

acmorrow · 2025-02-03T20:48:53Z

micro-rdk-nmea/src/messages/pgns.rs

@@ -106,3 +124,101 @@ pub struct GnssSatsInView {
    #[length_field = "sats_in_view"]
    satellites: Vec<Satellite>,
 }
+
+macro_rules! define_pgns {
+    ( $(($pgndef:ident, $enum:ident, $pgn:expr)),* ) => {


Do you still need $pgn?

micro-rdk-nmea-macros/src/attributes.rs

micro-rdk-nmea-macros/src/composition.rs

micro-rdk-nmea/src/messages/pgns.rs

acmorrow

LGTM with one last annoying question.

acmorrow · 2025-02-03T22:20:05Z

micro-rdk-nmea/src/messages/pgns.rs

-    (WaterDepth, Pgn128267, 128267),
-    (TemperatureExtendedRange, Pgn130316, 130316),
-    (GnssSatsInView, Pgn129540, 129540)
+    (WaterDepth, 128267),


Last question, I promise: Do you even need the PGN code as the second argument anymore?

yes I cannot use information only available at runtime to generate the content of the match statement for the from_bytes function

[CONSULT-469] - introduce macro for PGN registration and NmeaMessage …

a51e0d3

…type

gvaradarajan requested a review from acmorrow January 30, 2025 21:12

gvaradarajan requested a review from a team as a code owner January 30, 2025 21:12

gvaradarajan added 4 commits January 31, 2025 11:20

parse source id from data

d111cf0

Merge branch 'main' into nmea-parsing

b71f1cd

Merge branch 'main' into nmea-parsing

0dc686e

Merge branch 'main' into nmea-parsing

a8d97f6

acmorrow requested changes Jan 31, 2025

View reviewed changes

gvaradarajan added 2 commits January 31, 2025 15:48

address some review comments

a1687bb

missed one

d4c6798

gvaradarajan requested a review from acmorrow January 31, 2025 20:52

acmorrow reviewed Feb 3, 2025

View reviewed changes

micro-rdk-nmea/src/messages/pgns.rs Outdated Show resolved Hide resolved

acmorrow reviewed Feb 3, 2025

View reviewed changes

micro-rdk-nmea/src/messages/pgns.rs Outdated Show resolved Hide resolved

acmorrow reviewed Feb 3, 2025

View reviewed changes

pgn

5d942ee

acmorrow requested changes Feb 3, 2025

View reviewed changes

address review comments

71b1217

gvaradarajan requested a review from acmorrow February 3, 2025 22:12

acmorrow approved these changes Feb 3, 2025

View reviewed changes

Merge branch 'main' into nmea-parsing

3bde1c2

gvaradarajan merged commit fc41626 into viamrobotics:main Feb 4, 2025
6 checks passed

acmorrow mentioned this pull request Feb 4, 2025

PGN testing #411

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CONSULT-469] - introduce macro for PGN registration and NmeaMessage type #408

[CONSULT-469] - introduce macro for PGN registration and NmeaMessage type #408

gvaradarajan commented Jan 30, 2025 •

edited

Loading

acmorrow left a comment

acmorrow Jan 31, 2025

acmorrow Jan 31, 2025

acmorrow Jan 31, 2025

gvaradarajan Jan 31, 2025

acmorrow Jan 31, 2025

gvaradarajan Feb 3, 2025

acmorrow Jan 31, 2025

gvaradarajan Jan 31, 2025

acmorrow Jan 31, 2025

acmorrow Jan 31, 2025

gvaradarajan Feb 3, 2025

acmorrow Feb 3, 2025

gvaradarajan Feb 3, 2025

acmorrow Feb 3, 2025

acmorrow Jan 31, 2025

acmorrow Jan 31, 2025

acmorrow Feb 3, 2025

acmorrow left a comment

acmorrow Feb 3, 2025

acmorrow Feb 3, 2025

acmorrow Feb 3, 2025

acmorrow left a comment

acmorrow Feb 3, 2025

gvaradarajan Feb 3, 2025

		let metadata = NmeaMessageMetadata::try_from(value)?;
		let data = MessageData::from_bytes(metadata.pgn(), msg_data)?;

[CONSULT-469] - introduce macro for PGN registration and NmeaMessage type #408

[CONSULT-469] - introduce macro for PGN registration and NmeaMessage type #408

Conversation

gvaradarajan commented Jan 30, 2025 • edited Loading

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvaradarajan commented Jan 30, 2025 •

edited

Loading